fix: restrict client external navigation to http(s) and protocol-relative URLs#4
fix: restrict client external navigation to http(s) and protocol-relative URLs#4JaredStowell wants to merge 1 commit intomainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d1a4042e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| function isExternalUrl(href: string): boolean { | ||
| return /^[a-z][a-z0-9+.-]*:/i.test(href) || href.startsWith("//"); | ||
| return /^https?:\/\//i.test(href) || href.startsWith("//"); |
There was a problem hiding this comment.
Restricting isExternalUrl to only http(s) and // makes values like mailto:, tel:, and sms: fall into the internal navigation path, so useRouter().push()/replace() reaches history.pushState with a non-HTTP absolute URL and can throw SecurityError in browsers instead of delegating to native navigation. This is a regression from the previous behavior and breaks legitimate external protocol links; only dangerous schemes should be blocked.
Useful? React with 👍 / 👎.
| /** Check if a URL is external (http/https, or protocol-relative) */ | ||
| export function isExternalUrl(url: string): boolean { | ||
| return /^[a-z][a-z0-9+.-]*:/i.test(url) || url.startsWith("//"); | ||
| return /^https?:\/\//i.test(url) || url.startsWith("//"); |
There was a problem hiding this comment.
The same narrowing in the Pages Router isExternalUrl means next/router push/replace now treats mailto:/tel: URLs as internal, then executes window.history.pushState/replaceState on those values and can throw at runtime (or misroute when a basePath is set). This breaks existing external-protocol navigation semantics; the helper should still classify non-dangerous absolute schemes as external.
Useful? React with 👍 / 👎.
1 participant
Motivation
javascript:/data:URLs to be delegated towindow.location, creating a DOM XSS sink when untrusted input was used.Description
isExternalUrlin the Pages Router shim (packages/vinext/src/shims/router.ts) to return true only forhttp://,https://, or protocol-relative (//) URLs.packages/vinext/src/shims/navigation.ts) so both client navigation paths are consistent.tests/link.test.tsandtests/shims.test.tsthat explicitly expectjavascript:anddata:schemes to be treated as non-external.Testing
pnpm vitest run tests/link.test.ts -t "isExternalUrl"and they passed.pnpm vitest run tests/shims.test.ts -t "Pages Router router helpers"and they passed.pnpm vitest run tests/link.test.tsin this environment surfaced unrelated React hook/runtime test failures; the failures are unrelated to this change and the targeted assertions forisExternalUrlsucceed.Codex Task